Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Allow test projects to be SDK style #18810

Closed
wants to merge 49 commits into from
Closed

Conversation

chsienki
Copy link

@chsienki chsienki commented Jul 6, 2018

This change updates the associated targets and props files in the test directory to allow the test csproj's to be built as SDK style projects alongside traditional style projects.

It does not yet upgrade any of the projects, I'll submit a seperate batch of PRs for that, so that these changes aren't lost in a ton of mechanical refactoring.

While these changes allow the csproj's to be upgraded, there is still work to be done to allow depproj and ilproj's to be converted over to the new style. By allowing a mix of projects styles this change allows us to upgrade piece-meal rather than in a single big bang, hopefully allowing us to easier figure out bugs if they arise.

  • Remove CodeTaskFactory:
    • Allows the projects to be built using the core version of msbuild/dotnet build
    • Converted to using msbuild property expansion instead
  • Add directory.build.{props,targets}:
    • Currently we just import the dirs.props and targets, but means SDK style projects don't need to explicitly include these files
    • We probably want to move all projects over to using these in the future, but this keeps the changes smaller for now
  • Specific code for SDK projects:
    • There are a several changes required to build an SDK project. This change guards them behind conditionals so that only the new style projects see them. When we get to the point that there are only new projects, we can remove the guards (probably at the same time as ditching the dir.props)
  • Reordered build targets:
    • Because SDK projects implicitly import the build targets, we can no longer re-define the build targets unconditionally knowing they will likely be overwritten.
    • Instead we move the overwritten targets to seperate files, and include these conditionally based on properties. In this way there is always a build defined for SDK projects, which can then be overwritten to do nothing as needed.

Only pull in the buildtools targets for depproj, ilproj and non-sdk csproj
… targets for non-sdk projects, and only default to not building sdk projects when we're part of the overall build
@chsienki chsienki added test enhancement Improvements of test source code 2 - In Progress labels Jul 6, 2018
@chsienki
Copy link
Author

chsienki commented Jul 6, 2018

@RussKeldorph @weshaggard @AaronRobinsonMSFT

@sbomer I havn't tried merging your changes from #18695 yet. It looks like you moved some props stuff around, so there may be some adjustment needed there

Condition="'$(ReferenceSystemPrivateCoreLib)' == 'true' and '$(UsingMicrosoftNETSdk)' == 'true'"
AfterTargets="ResolveReferences">
<ItemGroup>
<ReferencePath Remove="@(ReferencePath)" Condition="$([System.String]::new('%(ReferencePath.HintPath)').EndsWith('System.Runtime.dll')) or $([System.String]::new('%(ReferencePath.HintPath)').EndsWith('System.Collections.dll'))" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these getting duplicated? Wouldn't it be better to try and eliminate the duplication instead of fixing it after the fact?

jashook and others added 8 commits July 9, 2018 09:13
The generated wrapper needs to target netcoreapp on unix. I had to
exclude assets from the xunit package and introduce a dependency on
the private corefx bits, to resolve a dependency conflict in which the
generated wrapper was depending on an older System.Runtime.dll than
the helper library.

I also disabled binclash logging, because the wrapper build binplaces
the helper library to the same location multiple times. I couldn't
find a simple way to disable binclash logging for the wrapper build
only, since that requires passing an empty switch to run.exe, and bash
word splitting makes this nontrivial from build-test.
Note that this will still require changes to the test wrapper
to actually source the TestEnv on unix
This way, the wrappers can build even if the 2.1 SDK isn't installed on
the machine.
When building wrappers using the SDK, we need some basic
properties (like the build os/arch/config, and the output directories)
to be set. I factored out properties used by both the old test build
and the new SDK-project test build.

At first I tried using Directory.Build.props (which is automatically
imported by the SDK), but our test build already imports SDK targets
in various places, so this was resulting in duplicate
imports. Instead, I used dir.common.props, and made the imports
explicit.
sbomer and others added 20 commits July 9, 2018 09:13
Use bash arrays to pass parameters for the build command. This makes
it possible to pass arguments with spaces to build_Tests_internal. We
use this to disable binclashlogging selectively (for the xunit wrapper
build only).
tests/src/dir.common.props was only used for the desktop-specific
xunit wrapper helper library. There's no need for it any more, so its
properties have been moved into tests/src/dir.props.

dir.sdkbuild.props has been renamed to dir.common.props, since it
contains properties used by SDK projects and buildtools projects.

This change also re-enables the test build.
With this, some properties shared by SDK projects can go in a global
location. The TargetFramework is shared by all SDK projects in the
test tree.

This change also uses a property for the xunit package directory that
contains the xunit.console.dll we copy to core_root.
This fixes a failure in the windows build.
On windows, the use of run.exe, config.json, and msbuild.cmd uses
msbuild.exe on the path. This change will build wrappers using the
local SDK via "dotnet msbuild", bypassing run.exe. Run.exe will go
away entirely with the move from buildtools to arcade, so other build
invocatios should follow suit.
UseBuildTools used to be true all the time. Now that we are building
wrappers on core, UseBuildTools becomes false. However, the rest of
the runtest.proj expects to build using buildtools, so we keep
UseBuildTools true until we switch to arcade.

The CSharpCoreTargetsPath was imported when running on core only. This
used to happen only on unix, but now it also happens when building
runtest.proj for the xunit wrappers on windows. On unix, this targets
file was a symlink to itself to work around some buildtools logic that
expected the file to exist. This workaround no longer appears
necessary, and on windows, this was never used in the first place, so
this change removes it.
UseRoslynCompilers was introduced in buildtools by
dotnet/buildtools#947, with different
behaviors on windows/unix. It was removed by
dotnet/buildtools#1974, so we can unify our
roslyn imports now.
…into merge_sven_xunit

# Conflicts:
#	tests/src/dir.common.props
@@ -36,7 +36,7 @@
<PgoDataPackageVersion>99.99.99-master-20180703-0030</PgoDataPackageVersion>
<MicrosoftNETCoreRuntimeCoreCLRPackageVersion>3.0.0-preview1-26704-01</MicrosoftNETCoreRuntimeCoreCLRPackageVersion>
<XunitPackageVersion>2.2.0-beta2-build3300</XunitPackageVersion>
<XunitConsoleNetcorePackageVersion>2.2.0-preview1-02830-02</XunitConsoleNetcorePackageVersion>
<XunitPackageVersion>2.4.0-beta.2.build4010</XunitPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove one of these 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this happened in my branch when I rebased. Thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have made that more clear. I merged @sbomer 's branch into this one, as we've both changed a bunch of stuff. I wonder if it's worth waiting for Sven's changes to land, then rebase onto that, so this is clearer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would make sense.


<!-- This file contains build properties that apply only to
SDK-style test projects. Properties that are shared between SDK
and buildtools projects should go in dir.common.props. -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this comment!!

@sbomer
Copy link
Member

sbomer commented Jul 19, 2018

@chsienki, I merged #18695. Let me know if I can help with the rebase.

@chsienki chsienki closed this Jul 19, 2018
@chsienki chsienki mentioned this pull request Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants